Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings#23627
Conversation
|
Hi @gabotechs, this is my first contribution to DataFusion. Would you be able to trigger CI and review this PR when you have a chance? I’m happy to explain any of the decisions I made here. I’m looking forward to being a useful member of the community. Thank you! |
gene-bordegaray
left a comment
There was a problem hiding this comment.
overall looks good, thanks for banging this one out, this is very very much appreaciated 🙇
One goal was to keep range_partitioning.slt a bit leaner. I think we can do some of that now by eliminating some repetitive tests. I was thnking:
- TEST 4: Exact Range Aggregate Below Subset Threshold
- TEST 5: Range Subset Aggregate Rehashes Below Subset Threshold
- TEST 6: Aggregate Rehashes Below Subset Threshold
- TEST 7: Aggregate Preserves Range When Preserve File Threshold Met
- TEST 8: Aggregate Rehashes When Preserve File Threshold Not Met
- TEST 13: Compatible Range Join Repartitions to Increase Parallelism
- TEST 14: Preserve File Partitions Preserves Range Join Inputs
cc: @gabotechs let me know what you think. This reduces the noise in that file to focus on range end to end and this unit now covers all the tricky "above this" "below that" distribution stuff 👍
| let plan = config.to_plan(requirement, &DISTRIB_DISTRIB_SORT); | ||
| let plan = displayable(plan.as_ref()).indent(true).to_string(); | ||
| let has_hash_repartition = | ||
| plan.contains("RepartitionExec: partitioning=Hash"); |
There was a problem hiding this comment.
I think we should be checking for any repartition. For example we can have a RoundRobin inserted if we choose to increase parallelism when less than target_partitions and threshld values
Which issue does this PR close?
Rationale for this change
The existing SQL logic tests exercise Range reuse through aggregates, joins, and windows. Repeating the same configuration permutations for every operator would make those tests large and difficult to maintain.
Issue #23572 describes six wildcard rules across four dimensions:
Those rules directly specify 15 of the 24 possible combinations. The remaining nine expectations are derived from the documented semantics rather than recorded from optimizer output:
range_satisfies_key_partitioning.allow_subset_satisfy_partitioning.This explains why these cases have different outcomes:
The equal partition count is not a global reuse shortcut. The input must first satisfy the distribution requirement. Exact keys satisfy directly, while subset keys require permission.
What changes are included in this PR?
The matrix uses one
RangeSatisfactionConfigCasestructure and eight configuration rows:Each row contains explicit expected results for exact, subset, and incompatible keys, producing all 24 combinations.
For every combination, the test:
The expected values remain explicit in the table rather than being calculated using a second implementation of the optimizer logic.
I also added
KeyPartitioningRequirementExecunder the existing physical-plan test utilities. It is a neutral single-input operator that requests keyed partitioning and enables the existing Range opt-in. This lets the matrix test the shared decision without aggregate, join, or window behavior affecting the result.I considered several test harnesses before using the neutral operator:
OutputRequirementExecand the existingMockReqExecrequest ordinary keyed distributions but cannot enable the crate-private Range opt-in.Are these changes tested?
Yes.
The following checks pass:
range_partitioning.sltfileMatrix mutation testing
The nine expectations not directly specified by the issue were mutation-tested against their decision branches:
datafusion/physical-plan/src/distribution_requirements.rs:459: changedPartitioningSatisfaction::ExacttoPartitioningSatisfaction::NotSatisfieddatafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1305: appended&& falsetoallow_subset_satisfy_partitioningdatafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1303: changedcurrent_partitions < target_partitionstocurrent_partitions <= target_partitionsThe final mutation changed one character and flipped only one matrix cell. This shows that the matrix distinguishes the preserve/target boundary rather than several cells merely depending on the same broad condition.
All mutations were restored, and the complete matrix passed again.
Operator opt-in mutation testing
I also independently removed each production Range opt-in and ran
range_partitioning.slt:AggregateExec—datafusion/physical-plan/src/aggregates/mod.rs:1952range_partitioning.slt:41,:101,:137,:218, and:551HashJoinExec—datafusion/physical-plan/src/joins/hash_join/exec.rs:1297range_partitioning.slt:280,:476,:514,:551, and:607BoundedWindowAggExec—datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:336range_partitioning.slt:701and:821WindowAggExec—datafusion/physical-plan/src/windows/window_agg_exec.rs:244range_partitioning.slt:730Each mutation introduced unexpected hash repartitions. Line 551 protects both
AggregateExecandHashJoinExec.This confirms that the two test layers protect different failure points:
When the SLT permutations are slimmed, the load-bearing operator cases above should remain. I would appreciate guidance on whether they should be marked as contract tests in place or moved into a dedicated section or file.
Are there any user-facing changes?
No query-planning or runtime behavior changes are included.
The only new exposed type is a test utility under the existing
datafusion_physical_plan::testmodule. I did not introduce a public Range-policy API because the current opt-in is documented as a temporary bridge pending #23266.